Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: make operators able to join all specified quorums #268

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

ian-shim
Copy link
Contributor

Why are these changes needed?

Currently, the operator registration logic will return without opting into specified quorums as long as the operator is registered to at least 1 quorum.
It should make sure the operator is registered to all specified quorums.
This PR also changes the signature of RegisterOperator method to take in churner client instead of taking churner URL. This makes it easier for the method to be tested and allows reusing the client instead of creating a new client every time.

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@ian-shim ian-shim marked this pull request as ready for review February 22, 2024 05:00
@@ -75,22 +62,22 @@ func RegisterOperator(ctx context.Context, operator *Operator, transactor core.T

privateKeyBytes := []byte(operator.KeyPair.PrivKey.String())
salt := [32]byte{}
copy(salt[:], crypto.Keccak256([]byte("churn"), []byte(time.Now().String()), operator.QuorumIDs[:], privateKeyBytes))
copy(salt[:], crypto.Keccak256([]byte("churn"), []byte(time.Now().String()), quorumsToRegister, privateKeyBytes))

// Get the current block number
expiry := big.NewInt((time.Now().Add(10 * time.Minute)).Unix())

// if we should call the churner, call it
if shouldCallChurner {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there could be another issue here. Suppose an operator has enough stake to enter quorum 0 but not enough for quorum 1, and both quorums are full. Right now, the operator will try to register for both quorums and be rejected by the churner.

We probably need to either:

  • Locally simulate the churner check and filter out quorums for which the operator will not be able to join. This is probably the best option, since there's no point in making a churner call if we can tell the operation will fail.
  • Update the churner so that it can handle partial failures.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this already fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This should be handled in a separate ticket.

Copy link
Collaborator

@mooselumph mooselumph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Noted one potential issue that probably needs to be addressed separately. I'll create a linear ticket for it.

@ian-shim
Copy link
Contributor Author

will close this PR for now to keep the current node cli behavior the same:
opt-in: register to EigenLayer and all specified quorums. This is called once per node
opt-out: completely deregister from all quorums. After this is called, the node is no longer operating in any quorums
update-quorums: updates the quorums that node is participating in. This can be called multiple times to register to new quorums or deregister from some quorums

@ian-shim ian-shim closed this Feb 26, 2024
@ian-shim ian-shim reopened this Mar 19, 2024
@ian-shim ian-shim force-pushed the operator-register-quorums branch from 1d25acf to 6d27ea6 Compare March 19, 2024 23:16
@ian-shim
Copy link
Contributor Author

will close this PR for now to keep the current node cli behavior the same: opt-in: register to EigenLayer and all specified quorums. This is called once per node opt-out: completely deregister from all quorums. After this is called, the node is no longer operating in any quorums update-quorums: updates the quorums that node is participating in. This can be called multiple times to register to new quorums or deregister from some quorums

We're redefining the cli behavior and removing update-quorums. It now needs to update opt-in so that it can opt in to additional quorums.

@jianoaix
Copy link
Contributor

merge master?

@ian-shim ian-shim force-pushed the operator-register-quorums branch 3 times, most recently from b71da90 to 821ffe9 Compare March 20, 2024 00:26
Copy link
Contributor

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall

@@ -75,22 +62,22 @@ func RegisterOperator(ctx context.Context, operator *Operator, transactor core.T

privateKeyBytes := []byte(operator.KeyPair.PrivKey.String())
salt := [32]byte{}
copy(salt[:], crypto.Keccak256([]byte("churn"), []byte(time.Now().String()), operator.QuorumIDs[:], privateKeyBytes))
copy(salt[:], crypto.Keccak256([]byte("churn"), []byte(time.Now().String()), quorumsToRegister, privateKeyBytes))

// Get the current block number
expiry := big.NewInt((time.Now().Add(10 * time.Minute)).Unix())

// if we should call the churner, call it
if shouldCallChurner {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this already fixed?

node/operator.go Show resolved Hide resolved
node/churner_client.go Show resolved Hide resolved
@ian-shim ian-shim force-pushed the operator-register-quorums branch from 821ffe9 to 275874d Compare March 21, 2024 00:13
@ian-shim ian-shim requested a review from jianoaix March 21, 2024 00:22
@ian-shim ian-shim merged commit 5078d4e into Layr-Labs:master Mar 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants